Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an option to terminate all existing connections to the database when resetting a database #239

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AustinShelby
Copy link

Description

This PR introduces a new --force option for the graphile-migrate reset command, which allows terminating existing connections to the database when dropping it.

I often face a case during end-to-end testing when before each test case I want to reset the database. Unfortunately due to some lingering connections from previous tests, the graphile-migrate reset command sometimes throws an error as expected. This change allows to bypass that limitation.

Performance impact

Unknown.

Security impact

Unknown.

Checklist

  • My code matches the project's code style and yarn lint:fix passes.
  • I've added tests for the new feature, and yarn test passes.
  • I have detailed the new feature in the relevant documentation.
  • I have added this feature to 'Pending' in the RELEASE_NOTES.md file (if one exists).
  • If this is a breaking change I've explained why.

);
if (force) {
await pgClient.query(
`DROP DATABASE IF EXISTS ${escapeIdentifier(databaseName)} WITH (FORCE);`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Introduced in PG13, so this is technically supported by all supported versions of PostgreSQL now. Interesting.

@benjie
Copy link
Member

benjie commented Mar 14, 2025

Thanks for the PR... I shall search my feelings 🤔

@benjie
Copy link
Member

benjie commented Apr 1, 2025

Sorry for the delay... Even now, as I write this comment, I'm going back and forth on how I feel about it.

It makes me feel uncomfortable... I don't really like the idea behind it. But also, it's for the reset command, so it's not a huge deal since you'd never do this in production. But also, all it effectively does is call pg_terminate_backend immediately before resetting the DB (I think?), which is something you could do yourself with a lifecycle hook (I think?). But also, most software's response to having the connection to the DB terminated is to reconnect, and reconnecting to a DB that's been dropped and recreated before the migrations have completed executing is not ideal and could lead to uncomfortable outcomes (e.g. code generation might run on an earlier state, PostGraphile watch mode would get inundated with DDL changes, etc causing high CPU but more importantly potentially high disk churn).

I guess my main concern is that this opens up a can of worms and may result in people raising further PRs after this (e.g. "lock the DB after reset to prevent X, Y and Z", "create the database with a different name and then rename it", etc) when really they shouldn't be running their application against a DB that's being reset - and if they weren't, they wouldn't need this PR.

@benjie
Copy link
Member

benjie commented Apr 1, 2025

Given your use case, I'd just recommend that you teach your test suite to be better at closing lingering connections (or you use pg_terminate_backend() yourself in the tests).

A technique that works really well for PostGraphile's test suite is to create a template test DB, then each test creates a clone of this template using a random name to use for that test and drops it at the end. The best part of this is it allows all my tests to run in parallel, the only downside of which is that the fans on my PC suddenly sound like a hurricane as all 32 cores are maxed out! (Cloning a template DB, assuming it doesn't have loads of data, is generally a sub-second operation.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants